Add verification of the length of the protobuf message #6070
Conversation
alexey-milovidov
changed the title
alexey-milovidov
added
the
pr-improvement
| @@ -67,7 +68,10 @@ bool ProtobufReader::SimpleReader::startMessage() | |||
| if (unlikely(in.eof())) | |||
| return false; | |||
| size_t size_of_message = readVarint(); | |||
| if (size_of_message == 0) | |||
| throwUnknownFormat(); | |||
There was a problem hiding this comment.
I don't think we should throw an exception in this case.
A message with all fields set by default doesn't seem to be incorrect.
| if (cursor < root_message_end) | ||
| return true; | ||
|
|
||
| throwUnknownFormat(); |
There was a problem hiding this comment.
@bopohaa I am not sure if this code is useful because I think there are enough checks for the end of the message in the functions SimpleReader::read* and we don't need one more check. Have you met a case when the ProtobufReader didn't throw an exception while it was reading invalid data?
There was a problem hiding this comment.
@bopohaa I am not sure if this code is useful because I think there are enough checks for the end of the message in the functions
SimpleReader::read*and we don't need one more check. Have you met a case when theProtobufReaderdidn't throw an exception while it was reading invalid data?
SimpleReader :: read * reads the entire stream without splitting it into separate messages. Therefore, it can read for quite a long time if the messages arrive with sufficient intensity.
There was a problem hiding this comment.
Now I understand your point. I agree it's better to detect errors as soon as possible. I've slightly changed your solution because it's faster to only compare cursor with field_end here (#6132)
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Category:
Short description:
Additional verification of the message length while reading data columns
Detailed description:
If the data format is broken (for example, serialization problems), then we should throw an exception if we read more bytes than specified in the message header.